Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch attributes from parent classes to allow reusability #581

Merged

Conversation

bastien-phi
Copy link
Contributor

This PR changes class attributes resolution fetching parent classes attributes as well as current class attributes.

This allows the developer to factorize attributes declarations parent classes.

For example if we want every data to be mapped with SnakeCaseMapper, we currently need to declare #[MapName(SnakeCaseMapper::class)] on every single Data class.

With this PR, we can declare a BaseData with the mapper and every child class with inherit with the mapper.

#[MapName(SnakeCaseMapper::class)]
abstract class BaseData extends Data {}

class MyData extends BaseData {
    public function __construct(
        public string $firstName,
    ) {}
}

MyData::from(['first_name' => 'John']); // work as expected

In case the attribute is redeclared in child class, the child attribute has higher priority (parents attributes are push at the end of the collection).

Copy link
Member

@rubenvanassche rubenvanassche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, some small changes required

tests/Support/DataClassTest.php Outdated Show resolved Hide resolved
src/Support/DataClass.php Show resolved Hide resolved
->map(fn (ReflectionAttribute $reflectionAttribute) => $reflectionAttribute->newInstance());

$parent = $class->getParentClass();
if ($parent !== false && $class->getName() !== Data::class) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to check on the BaseData interface here, since in v4 we'll have mutliple base Data classes like Dto and Resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided not to exclude base class provided by the package here. Easier way, not sure if we can exclude base class based on an interface it implements as the child class will also implements this interface...

@bastien-phi
Copy link
Contributor Author

@rubenvanassche This should be better

@rubenvanassche
Copy link
Member

Looking great! Thanks!

@rubenvanassche rubenvanassche merged commit 1d17ac5 into spatie:main Oct 12, 2023
11 checks passed
@bastien-phi
Copy link
Contributor Author

Thank you !

@bastien-phi bastien-phi deleted the allow_attributes_from_parent_classes branch October 12, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants